-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: Added bash script for updating data.yaml
#2769
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This issue is currently awaiting triage. If kube-state-metrics contributors determine this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Rishab87 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
8c35fe7
to
92ca55e
Compare
scripts/update-data-yaml.sh
Outdated
echo "Fetching latest Kubernetes version..." | ||
LATEST_K8S_FULL_VERSION=$(curl --silent --fail "https://api.github.com/repos/kubernetes/kubernetes/releases/latest" | jq -r '.tag_name') | ||
|
||
if [ -z "$LATEST_K8S_FULL_VERSION" ] || [ "$LATEST_K8S_FULL_VERSION" == "null" ]; then | ||
echo "Error: Failed to fetch the latest Kubernetes version from GitHub." >&2 | ||
exit 1 | ||
fi | ||
|
||
LATEST_K8S_VERSION=$(echo "${LATEST_K8S_FULL_VERSION}" | sed 's/^v//' | cut -d. -f1,2) | ||
echo "Latest stable Kubernetes version (N): ${LATEST_K8S_VERSION}" | ||
|
||
K8S_MAJOR=$(echo "${LATEST_K8S_VERSION}" | cut -d. -f1) | ||
K8S_MINOR=$(echo "${LATEST_K8S_VERSION}" | cut -d. -f2) | ||
PREV_K8S_MINOR=$((K8S_MINOR - 1)) | ||
K8S_VERSION_FOR_NEW_RELEASE="${K8S_MAJOR}.${PREV_K8S_MINOR}" | ||
echo "New release ${NEW_VERSION_WITH_V} will be mapped to Kubernetes (N-1): ${K8S_VERSION_FOR_NEW_RELEASE}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should check k8s.io/client-go's version in go.mod when bumping, if I understand it correctly. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup right, changed it
92ca55e
to
97387e7
Compare
scripts/update-data-yaml.sh
Outdated
|
||
check_command "yq" | ||
check_command "jq" | ||
check_command "curl" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we don't need curl anymore. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup
scripts/update-data-yaml.sh
Outdated
check_command "yq" | ||
check_command "jq" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use github.com/itchyny/gojq/cmd/gojq instead?
Once #2660 is in, it will be used anyway as part of the project. It supports yaml via --yaml-input --yaml-output args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this, havent pushed go.mod
and go.sum
since this is to be merged after that PR
scripts/update-data-yaml.sh
Outdated
exit 1 | ||
fi | ||
|
||
CLIENT_GO_FULL_VERSION=$(grep 'k8s.io/client-go' "${GO_MOD_FILE}" | awk '{print $2}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get the version like this: https://github.com/kubernetes/kube-state-metrics/pull/2739/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R23
97387e7
to
c831c83
Compare
@mrueg I've addressed all the reviews, can you please re-review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, generally I'd like to see the more complex and lengthy pipe-based commands here broken down and commented at each step (concisely) so they're easier to maintain.
-FOO=$(a|b|c)
+FOO=$(\
+# what "a" does
+ a|\
+# what "b" does
+ b|\
+# what "c" does
+ c\
+)
scripts/update-data-yaml.sh
Outdated
echo "${JSON_DATA}" | gojq -r --arg version "${NEW_VERSION_WITH_V}" --arg k8s "${K8S_VERSION_FOR_NEW_RELEASE}" ' | ||
.compat[] | select(.version != $version) | " - kubernetes: \"" + .kubernetes + "\"\n version: " + .version | ||
' >> "${DATA_FILE}" | ||
|
||
echo " - kubernetes: \"${K8S_VERSION_FOR_NEW_RELEASE}\"" >> "${DATA_FILE}" | ||
echo " version: ${NEW_VERSION_WITH_V}" >> "${DATA_FILE}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I maybe off but wouldn't append main
to the list as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it's being appended separately below
scripts/update-data-yaml.sh
Outdated
VERSIONS_LIST=$(echo "${JSON_DATA}" | gojq -r '.compat[] | select(.version != "main") | "\(.version)|\(.kubernetes)"' 2>/dev/null || true) | ||
FULL_VERSIONS_LIST=$(printf "%s\n%s|%s" "${VERSIONS_LIST}" "${NEW_VERSION_WITH_V}" "${K8S_VERSION_FOR_NEW_RELEASE}") | ||
|
||
SORTED_VERSIONS=$(echo "${FULL_VERSIONS_LIST}" | grep -v '^$' | sort -t'|' -k1,1 -Vr | head -n "${NUM_RELEASES_TO_KEEP}" | sort -t'|' -k1,1 -V) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I maybe off here since I've not run this, but wouldn't this take into account the current 5 releases, instead of dropping the oldest one, and appending the given release and then main
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it takes into account current 5 releases excluding the main, so it removes the oldest release, adds the new one and updates the main version too if needed
c831c83
to
f564d13
Compare
f564d13
to
a098852
Compare
a098852
to
37d117a
Compare
scripts/update-data-yaml.sh
Outdated
K8S_MINOR=$(echo "${CLIENT_GO_FULL_VERSION}" | cut -d. -f2) | ||
|
||
K8S_VERSION_FOR_NEW_RELEASE="1.${K8S_MINOR}" | ||
echo "New release ${NEW_VERSION_WITH_V} will be mapped to Kubernetes (N-1): ${K8S_VERSION_FOR_NEW_RELEASE}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean with the n-1 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added that by mistake I was thinking something while writing down this script, removing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's supposed to mean the latest-1 release, relative to the latest KSM release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think so I added it earlier because I was subtracting one version behind the latest k8s version but later Manuel suggested to take the k8s version from go.mod
instead of fetching from github api and subtracting it.
scripts/update-data-yaml.sh
Outdated
echo "New release ${NEW_VERSION_WITH_V} will be mapped to Kubernetes (N-1): ${K8S_VERSION_FOR_NEW_RELEASE}" | ||
|
||
|
||
JSON_DATA=$(cat "${DATA_FILE}" | gojq -r --yaml-input '.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to use cat or can gojq also read from a file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, gojq can read directly from a file without using cat. I've already removed cat from here
scripts/update-data-yaml.sh
Outdated
EXISTING_EXACT_MATCH=$(\ | ||
echo "${JSON_DATA}" |\ | ||
# Query for existing entry with same version and k8s mapping | ||
gojq -r --arg version "${NEW_VERSION_WITH_V}" --arg k8s "${K8S_VERSION_FOR_NEW_RELEASE}" ".compat[] | select(.version == \$version and .kubernetes == \$k8s) | .version // empty"\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can call gojq via go tool similar to how we do it in the makefile
37d117a
to
7f9ca0f
Compare
@mrueg I've addressed all the reviews, can you please re-review it? |
@@ -0,0 +1,152 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this file isn't executable by default, can you run chmod +x and commit it again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
7f9ca0f
to
4344ef3
Compare
scripts/update-data-yaml.sh
Outdated
# Remove empty lines | ||
grep -v '^$' |\ | ||
# Sort by version (descending) using version sort | ||
sort -t'|' -k1,1 -Vr |\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to sort twice here? Could we use tail instead of head?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup you're right, thanks for pointing it out made the changes
4344ef3
to
84f24be
Compare
|
||
SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd ) | ||
REPO_ROOT=$( cd -- "${SCRIPT_DIR}/.." &> /dev/null && pwd ) | ||
DATA_FILE="${REPO_ROOT}/data.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about maintaining a couple of mock data files (inputs and outputs), and populating them with the various use-cases covered here, to ensure functionality doesn't break over time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah nice idea, added them
84f24be
to
a77dd36
Compare
@rexagod I've addressed all your reviews, can you please re-review it? |
@rexagod just a follow up on this one. |
What this PR does / why we need it:
This PR automates the process of updating the
data.yaml
file. To run specify the new version like this:For each new version, it fetches the latest Kubernetes releases (we keep the k8s version one behind the latest release), adds the new compatibility mapping, and prunes the oldest entry. This ensures our compatibility data is kept current automatically with each new release.
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
does not change
Which issue(s) this PR fixes: (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged)Related to #2756